-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use SO_REUSEPORT_LB on FreeBSD #106
use SO_REUSEPORT_LB on FreeBSD #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this on an actual FreeBSD?
control_unix.go
Outdated
if err != nil { | ||
return | ||
} | ||
err = unix.SetsockoptInt(int(fd), unix.SOL_SOCKET, FREEBSD_SO_REUSEPORT_LB, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be executed on FreeBSD. We can't just set a random sock opt on other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I was thinking of making it future proof by having it for all unix systems.
But if these macros are defined per OS, then it makes sense to protect it as per runtime.
Will change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constant is only defined on FreeBSD. You'll need to use build flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, tested on freeBSD and macOS after these changes.
Installed a freeBSD VM and tested on it after addressing review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the changes except for the change in build flags are needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah..right, Thanks for your patience man.
Needed a lot of handholding for this one .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. The fact that I have to approve CI every time makes things more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one thing i wanted to find out, can't this be left to run automatically?
Would save your time and turn-around time for public contributors as well, because if there are any issues we can fix them based on CI failures itself.
gofmt is unhappy. |
Ah, one of those times i used vim and not vscode. |
Added new socket option supported in freeBSD systems as per #105
Tested in macOS only. Needs to be tested on a freeBSD system to actually verify if this option is working fine or not.